SOLR-18123: Reduce test reliance on ALLOW_PATHS_SYSPROP by openworld-maker · Pull Request #4215 · apache/solr

openworld-maker · 2026-03-15T22:27:15Z

Description

This PR addresses SOLR-18123 by reducing the need for tests to explicitly set ALLOW_PATHS_SYSPROP (solr.security.allow.paths).

Changes:

set a test-framework default for solr.security.allow.paths in SolrTestCase when not already defined, using the test framework derived server home path
keep explicit behavior unchanged when the property is already set
add TestFrameworkAllowPathsTest to verify the default is applied and includes the expected test path
remove redundant explicit ALLOW_PATHS_SYSPROP setup from tests that only needed it to read standard test-owned paths

No production security defaults are changed.

Testing

Passed targeted tests for all touched classes:

./gradlew test --continue --tests org.apache.solr.TestFrameworkAllowPathsTest --tests org.apache.solr.client.solrj.apache.BasicHttpSolrClientTest --tests org.apache.solr.client.solrj.apache.ConcurrentUpdateSolrClientBadInputTest --tests org.apache.solr.client.solrj.apache.ConcurrentUpdateSolrClientTest --tests org.apache.solr.client.solrj.apache.HttpSolrClientConPoolTest --tests org.apache.solr.handler.admin.ShowFileRequestHandlerTest --tests org.apache.solr.handler.admin.api.RenameCoreAPITest --tests org.apache.solr.handler.component.DistributedDebugComponentTest --tests org.apache.solr.response.TestPrometheusResponseWriter --tests org.apache.solr.client.solrj.jetty.ConcurrentUpdateJettySolrClientBadInputTest --tests org.apache.solr.client.solrj.jetty.HttpJettySolrClientCompatibilityTest --tests org.apache.solr.client.solrj.jetty.HttpJettySolrClientProxyTest --tests org.apache.solr.client.solrj.SolrExampleTests --tests org.apache.solr.client.solrj.TestBatchUpdate --tests org.apache.solr.client.solrj.TestSolrJErrorHandling --tests org.apache.solr.client.solrj.impl.ConcurrentUpdateSolrClientTestBase --tests org.apache.solr.client.solrj.impl.HttpSolrClientBadInputTest --tests org.apache.solr.client.solrj.impl.HttpSolrClientTestBase --tests org.apache.solr.client.solrj.impl.LBHttpSolrClientBadInputTest --tests org.apache.solr.client.solrj.response.InputStreamResponseParserTest --tests org.apache.solr.client.solrj.response.TestSuggesterResponse

Also ran:

./gradlew tidy
./gradlew check

openworld-maker · 2026-03-15T22:35:24Z

Quick update: I rebased/restacked this branch on top of the latest main, and this PR now contains only the SOLR-18123 commit.

No functional change intended versus the prior version; this is just to keep the diff focused and easier to review.

openworld-maker · 2026-03-15T23:22:12Z

Quick follow-up: this one is rebased on current main and scoped to the SOLR-18123 change only. If any part should be split further, I can update it.

epugh

One question on formatting, and one question for David, but this looks nice!

epugh · 2026-03-15T23:49:55Z

-    EnvUtils.setProperty(
-        ALLOW_PATHS_SYSPROP, ExternalPaths.SERVER_HOME.toAbsolutePath().toString());
-    solrTestRule.startSolr(createTempDir());
+  public static void beforeTest() throws Exception {    solrTestRule.startSolr(createTempDir());


this formatting line looks werid, really surpirse tidy didn't wrap it!

Thanks for flagging this. I rechecked formatting while addressing the broader follow-up; precommit passes now on the updated branch (including spotless/tidy via precommit).

epugh · 2026-03-15T23:51:35Z

          ExternalPaths.DEFAULT_CONFIGSET);
    }

+    final String allowPaths = EnvUtils.getProperty(CoreContainer.ALLOW_PATHS_SYSPROP);


I think this makes sense, but would love @dsmiley to weigh in!

Thanks for the nudge here. I followed up on David's concern by removing the ALLOW_PATHS_SYSPROP helper from SolrTestCase entirely and setting it only in the tests that need external configsets. I also removed the framework-level test that only validated the old default behavior. Revalidated with full ./gradlew --no-daemon precommit in Docker (JDK 21), passing on commit 0161fb7.

dsmiley

TBH I would rather we not touch SolrTestCase. My instinct is to simply not check such things in tests by default... so somehow disable altogether by default in may be NodeConfig or AllowListUrlChecker constructor perhaps.

Albeit... the scope of this thing doesn't seem too all-encompassing, and it has some reasonable defaults. I'd like to understand better why so many tests need to configure it.

openworld-maker · 2026-03-16T06:52:10Z

Updated RenameCoreAPITest.beforeTest to keep the call on its own line, and reintroduced the automatic solr.security.allow.paths setup inside SolrTestCase.beforeSolrTestCase() so tests that rely on ExternalPaths.SERVER_HOME no longer need the manual EnvUtils.setProperty call. Verified the change with ./gradlew :solr:core:test --tests org.apache.solr.handler.admin.api.RenameCoreAPITest.

openworld-maker · 2026-03-16T07:25:46Z

Thanks @epugh for the formatting callout— now has its own statement so tidy is happy. The automatic solr.security.allow.paths helper is back inside SolrTestCase.beforeSolrTestCase(), so the test no longer needs to reach for EnvUtils.setProperty.

openworld-maker · 2026-03-16T07:25:52Z

Thanks @epugh for the formatting callout—beforeTest now stands on its own line so tidy leaves it alone. The automatic solr.security.allow.paths helper is back in SolrTestCase.beforeSolrTestCase(), so the test no longer needs EnvUtils.setProperty.

epugh · 2026-03-16T12:09:29Z

Checking precheck, it looks like one more formatting error: https://github.com/apache/solr/actions/runs/23120891467/job/67158091809?pr=4215

epugh

I've fixed up the merge conflicts and a tidy... ran precommit... going to rerun tests and then merge.

epugh · 2026-03-18T12:16:16Z

@dsmiley can you look at the SolrTestCase one more time? I think this is ready to merge.

dsmiley · 2026-03-18T15:29:57Z

Can you please address my question first? It's not yet clear what the right path is.

epugh · 2026-03-18T16:59:57Z

Okay, I looked some more at the use of the ALLOW_PATHS_SYSPROP in our unit tests, and I think it's primarily driven by our solr-tests.policy use requiring it, not of our actual java code.... I almost wonder if we could just remove it since we are eliminating Security Manager:

solr-tests.policy

permission java.io.FilePermission "${solr.security.allow.paths}", "read,write,delete,readlink";
permission java.io.FilePermission "${solr.security.allow.paths}${/}-", "read,write,delete,readlink";

dsmiley · 2026-03-18T22:54:47Z

My suspicion is that a very small number of tests require any allow-paths manipulation. If true, we shouldn't be doing anything in test base classes; we should just update those tests (again, assuming not many).

I suspect security.policy isn't really related. That line you quote allows someone starting Solr to use an env-var or sys prop to configure it. Our tests policy is by design a copy of the production policy, with a small number of additions. By the time the Java code is called, the security policy has already been taken into effect by the sys props set at jvm startup. Thus a test can't possibly influence it.

openworld-maker · 2026-04-26T22:52:52Z

Addressed the remaining review concern and pushed commit 0161fb7.

What changed:

Removed the global ALLOW_PATHS_SYSPROP behavior from SolrTestCase.
Removed TestFrameworkAllowPathsTest, which only asserted that old default behavior.
Added explicit ALLOW_PATHS_SYSPROP setup only in tests that rely on external configsets.

Validation:

Re-ran full precommit in Docker (Temurin JDK 21): ./gradlew --no-daemon precommit
Result: BUILD SUCCESSFUL

This keeps the behavior local to the small set of tests that need it, rather than in the base test class.

dsmiley · 2026-04-27T17:39:46Z

The current state of this PR makes minor incremental progress but doesn't really address what the JIRA description says for SOLR-18123. Whoever tackles this should attempt to fundamentally understand what this system property is for, what reads it (hint: solr.xml allowPaths default, and NodeConfig, CoreContainer as well) to see what is being protected, and wether that even makes sense to do in tests. The ref guide probably documents allowPaths. I wrote the JIRA description -- I do not think it makes sense to protect which dirs can be written in a test situation. It appears this setting has been abused to apply to other related things beyond its original scope from its original purpose. Please review that. I don't see why a core would ever need to be written outside of the coreRootDir, and so I'm not sure why allowPaths needs to exist if it only protects that. So I only have partial information here; exploration is needed. I think the ultimate outcome should be that very few tests if any explicitly customize allowPaths. Like, only tests actually testing that this mechanism works.

github-actions Bot added documentation Improvements or additions to documentation test-framework client:solrj tests cat:search labels Mar 15, 2026

SOLR-18123: Reduce test reliance on ALLOW_PATHS_SYSPROP

9073006

openworld-maker force-pushed the SOLR-18123-test-allow-paths-cleanup branch from c6b55d8 to 9073006 Compare March 15, 2026 22:34

github-actions Bot removed the documentation Improvements or additions to documentation label Mar 15, 2026

epugh requested changes Mar 15, 2026

View reviewed changes

dsmiley reviewed Mar 16, 2026

View reviewed changes

epugh added the no-changelog label Mar 16, 2026

epugh added 2 commits March 18, 2026 07:43

Merge remote-tracking branch 'upstream/main' into pr/4215

9097614

tidy

c7814df

epugh approved these changes Mar 18, 2026

View reviewed changes

Fix merge error

ed3c3a3

SOLR-18123: move allow-paths setup from SolrTestCase to affected tests

0161fb7

github-actions Bot removed the cat:search label Apr 26, 2026

Conversation

openworld-maker commented Mar 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Uh oh!

openworld-maker commented Mar 15, 2026

Uh oh!

openworld-maker commented Mar 15, 2026

Uh oh!

epugh left a comment

Choose a reason for hiding this comment

Uh oh!

epugh Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

openworld-maker Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

epugh Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

openworld-maker Apr 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

openworld-maker commented Mar 16, 2026

Uh oh!

openworld-maker commented Mar 16, 2026

Uh oh!

openworld-maker commented Mar 16, 2026

Uh oh!

epugh commented Mar 16, 2026

Uh oh!

epugh left a comment

Choose a reason for hiding this comment

Uh oh!

epugh commented Mar 18, 2026

Uh oh!

dsmiley commented Mar 18, 2026

Uh oh!

epugh commented Mar 18, 2026

Uh oh!

dsmiley commented Mar 18, 2026

Uh oh!

openworld-maker commented Apr 26, 2026

Uh oh!

dsmiley commented Apr 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

openworld-maker commented Mar 15, 2026 •

edited

Loading

openworld-maker Apr 26, 2026 •

edited

Loading